Split ev_op/init#526
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the eko.evolution_operator package by extracting the Numba-based quadrature kernel implementation (quad_ker, QuadKerBase, and related helpers) out of evolution_operator/__init__.py into a dedicated quad_ker.py module, and updates internal imports accordingly.
Changes:
- Move the quadrature-kernel logic (including
QuadKerBaseand QCD/QED kernel builders) fromevolution_operator/__init__.pyintoevolution_operator/quad_ker.py. - Slim down
evolution_operator/__init__.pyby removing large Numba-heavy definitions and importingquad_kerfrom the new module. - Update
operator_matrix_element.pyto importQuadKerBasefromquad_ker.pyinstead of from the package__init__.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/eko/evolution_operator/quad_ker.py | Hosts the extracted quadrature-kernel code, adds/relocates helper selectors and QuadKerBase, retains Numba cfunc callbacks. |
| src/eko/evolution_operator/operator_matrix_element.py | Adjusts imports to reference QuadKerBase from the new quad_ker module. |
| src/eko/evolution_operator/init.py | Removes the inlined quad-kernel implementation and imports quad_ker from quad_ker.py. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
So apparently because of the three Unfortunately there is no dedicated import os
if os.environ.get("NUMBA_DISABLE_JIT", "0") == "1":
def cfunc_wrapper(sig, *args, **kwargs):
def decorator(func):
return func
return decorator
nb.cfunc = cfunc_wrapperPlease do let me know if you are satisfied with this or not so that I can start fixing patch files for |
Actually, I would say this is a feature and not a bug.
Do you agree? (and to be explicit: Copilot is telling non-sense in this case I would say) Then there is the unrelated matter of Numba+caching: this yields sometimes strange behaviour since the caching is not always consistent. See also numba/numba#8926 . Thus, the first action in debugging Numba is always to delete the (Python) cache (files) and let Numba recompile; typically it is sufficient to delete "the relevant cache" (which you need to guess) and not all, which for us can take a very long time to recompile. |
|
In summary, today, I removed I also changed poe compile to use maturin, added that as a dev-dependency. This led to changes in |
| /// - [`qcd_gamma_singlet`]: QCD singlet anomalous dimension matrix in Mellin space | ||
| #[pymodule] | ||
| fn ekors(_py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> { | ||
| m.add_function(wrap_pyfunction!(qcd_gamma_singlet, m)?)?; |
There was a problem hiding this comment.
If this works, please also add the non-singlet counterpart
| [dependencies] | ||
| num = "0.4.1" | ||
| ekore = { path = "../ekore", version = "0.0.1" } | ||
| pyo3 = { version = "0.21", features = ["extension-module"] } |
There was a problem hiding this comment.
Can you please briefly explain to me what the extension-module does?
There was a problem hiding this comment.
So when I first did poe compile with maturin, it gave me this warning:
⚠️ Warning: You're building a library without activating pyo3's `extension-module` feature. See https://pyo3.rs/v0.21.2/building-and-distribution.html#the-extension-module-feature
Without extension-module, a pyo3 package links its own libpython into the .so. But when Python imports our package, there's already a libpython running, the Python interpreter itself. Having two copies of libpython in the same process causes conflicts. extension-module disables this. It tells the package to just use the libpython that's already running instead of bundling its own.
The downside is that cargo test will break for any pyo3 functions, since the test binary runs standalone and has no libpython to rely on. This shouldn't be an issue for now as we don't plan to write tests for these.
Also, as per the docs linked in the warning:
Python extensions on Unix must not link to libpython for manylinux compliance.
So extension-module is a necessity here.
Do tell me if you are satisfied by reacting with an emoji so that I can resolve the conversation :)
There was a problem hiding this comment.
of course we need to counteract this deletion (which is the right thing), with the corresponding new patch
| + order_qcd = order[0] | ||
| + n_re = ker_base.n.real | ||
| + n_im = ker_base.n.imag | ||
| + with nb.objmode(gamma_singlet="complex128[:,:,:]"): |
There was a problem hiding this comment.
I guess the problem is not so much calling a foreign function in numba, but rather a PyO3 object ...
For the context of #136 (which will introduce another C library into the mess of anomalous dimensions), I was experimenting with how to do that and for plain C functions it seems to be straight:
from numba import njit
import ctypes
DSO = ctypes.CDLL('./shared_library.so')
# Add typing information
c_func = DSO.sum
c_func.restype = ctypes.c_int
c_func.argtypes = [ctypes.c_int, ctypes.c_int]
@njit
def example(x, y):
return c_func(x, y)
print(example(3, 4)) # 7// gcc lib.c -fPIC -shared -o shared_library.so
#include <stdint.h>
int64_t sum(int64_t a, int64_t b){
return a + b;
}here we don't have a direct C library in hand, so it does not really help us, right?
well, actually it shows another alternative to PyO3: we could add a C interface also to crates/eko (remember that the target of #519 is crates/ekore) and use that under scipy ... in the end we don't care so much crates/eko being a Python library since it is purely internal (since it is just a glue) ....
any thoughts/opinions?
what is the easiest way? which we can grow smoothly? if we ever would want to change strategy, we most likely want to do so in a next step, provided objmode works ...
There was a problem hiding this comment.
The issue with using ctypes was passing tuples, lists, and arrays. There was some sort of type mismatch between python and Rust. In the end I just gave up. I thought to myself that all of this is just temporary, let us make a working solution, because in the future we will remove the #[pyfunction] from qcd_quad_ker, and add that macro to the quad_ker function. Ultimately we got to remove this #[pyfunction] entirely and directly pass a single C-ABI function to scipy.integrate.quad with LowLevelCallable.
For #519, I thought you wanted to make that crate to expose the ekore crate to other C/C++ projects and not the python one. The quad function requires almost half of the src/eko python code to be a part of it's hot loop. We can try to add them in the eko_capi, but I don't think we should should change the whole structure of the crate for just one function. Otherwise, we will have call that one function from eko crate, expose it as C-ABI in eko_capi, this will bloat the eko_capi crate as now eko crate will be required for the compilation. After that use the .so file for just one function while it exposes other functions too, again feels bloated to me.
It's better to stick to what we are currently doing.
| run: pip install poethepoet | ||
| - name: Run benchmark | ||
| env: | ||
| VIRTUAL_ENV: ${{ env.pythonLocation }} |
There was a problem hiding this comment.
poe compile did not work here because maturin was not able to identify the virtual environment. This was the root cause told by the error:
For maturin to find your virtualenv you need to either set VIRTUAL_ENV or have a virtualenv called .venv
GitHub Actions' setup-python installs Python at pythonLocation (i.e. /opt/hostedtoolcache/Python/3.12.13/x64) but does not set up a virtual environment. So I pointed VIRTUAL_ENV to pythonLocation, which is already set by setup-python, so maturin can find the correct Python to build against.
Do tell me if you are satisfied by reacting with an emoji so that I can resolve the conversation :)
a step for #518
src/eko/evolution_operator/__init__.pyinto two parts: the new part (quad_ker.py) containing the integration kernels only and the remaining part containing only the class(EDIT: this would have been better placed in a comment, since it is not describing the PR but rather one specific problem)
@felixhekhorn @scarlehoff I have one small issue with this and hence I'd like to now if you are facing this or not. So after changing all the files I tried to run poe test and got this error:
In short, the function
select_singlet_elementis of typefunction, and not whatever numba wants it to be (i.e. njit function). I have no idea why this is taking place since the function is placed above in the file, and the structure of the file before was the same only but it did not encounter this error. I ran this and the error got fixed:So remove all the cache, and then call only the quad_ker file such that it caches without any problems. Then running
poe testdoes not result in any error.I had faced this error before. After running rustify.sh, poe test was returning the same error. I imported all the dependencies of quad_ker.py in terminal (similar way as shown above) and the error was resolved.
I have no clue why on Earth numba is showing this behaviour, but we should not split the file if the error persists.